Codecov Report

❌ Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.68%. Comparing base (506c89b) to head (216f80a).

❌ Your patch status has failed because the patch coverage (86.11%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (99.68%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
- Coverage   99.96%   99.68%   -0.29%     
==========================================
  Files          64       64              
  Lines        2789     2819      +30     
==========================================
+ Hits         2788     2810      +22     
- Misses          1        9       +8     

@d-v-b this is a 'minimised' version of your PR, which uses the existing test infastructure instead of introducing pixi, which can be done in an orthogonal PR. Perhaps you could a review, and then we can get numcodecs development up and running again.

can we run these parametrized tests locally?

You can manually set up an environment with the suitable version of zarr-python, and run the tests from there.

and that for me is a pretty meaningful downgrade from #766, which makes it really easy to run the parametrized tests locally.

It's not possible to run parameterized tests against e.g., different versions of Python at the moment, so I'd say that's beyond the scope of just fixing numcodecs with Zarr-python 3.1.x. My thinking was limiting the scope would make for a quicker review, instead of us also having to work out the best way to do local paramterised tests which is another kettle of fish.

If we fix a bug, we need to make sure we properly test for it. We have a bug exposed by different versions of zarr python, so we need an easy way to test against different versions of zarr python. We don't need the best way to do local parametrized tests. we just some way.

I don't think pixi and hatch are controversial tools, and they are totally opt-in. numcodecs developers don't have to use them if they don't want to.

IMO we should just polish #766 instead of settling for something that's a worse local dev experience.

If we fix a bug, we need to make sure we properly test for it.

We are testing for it, I added a test run with zarr-python 3.0.x

We don't need the best way to do local parametrized tests. we just some way.

That some way is making a virtual environment with different zarr-python versions

settling for something that's a worse local dev experience.

It's not a worse local dev experience, it's the same as before where you had to manually make a new environment to test.

It's not a worse local dev experience, it's the same as before where you had to manually make a new environment to test.

It should be easy to test the new functionality added here. Manually making a new environment to test this kind of thing is not easy. And the test you added runs in CI, not locally. So yes, this PR represents a worse local dev experience than #766, which lets you use hatch to run parametrized tests if you want.

Manually making a new environment to test this kind of thing is not easy.

It might not be the easist way to do it, but it's the status quo way to test anything locally at the moment.

So yes, this PR represents a worse local dev experience than #766, which lets you use hatch to run parametrized tests if you want.

The new features in #776 are great, and I agree an improvement over this PR and the status quo, but this PR does not make testing locally any harder than it was before.

I don't understand why improving the devloper experience has to hold up a bugfix...

It might not be the easist way to do it, but it's the status quo way to test anything locally at the moment.

There is nothing special about the status quo. The status quo got us the bug! #766 attempts to change the status quo, for good reason, so that bugs are less frequent.

I don't understand why improving the devloper experience has to hold up a bugfix...

Because we already have an open PR (#766) that fixes the bug, AND makes them less likely to occur in the future, by making the relevant tests easier to run. This is the value of improving the developer experience -- fewer bugs.

The new features in #776 are great, and I agree an improvement over this PR and the status quo

so what are we doing in this pr, instead of getting #776 out the door?

🤷 I'm trying to get the bugfix released, by limiting the scope of the PR to just the bugfix and CI needed to test it. From my point of view I don't know why there's a blocker on fixing the bug here and without other improvements, but we just seem to disagree, so I'm going to close this.

🤷 I'm trying to get the bugfix released, by limiting the scope of the PR to just the bugfix and CI needed to test it.

Where in #766 does anyone say the scope is too broad?